Skip to content

Package pplumbing.0.0.13 #27912

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from

Conversation

mbarbin
Copy link
Contributor

@mbarbin mbarbin commented May 22, 2025

pplumbing.0.0.13

Utility libraries to use with pp
[pplumbing] defines a set of utility libraries to use with pp. It
is compatible with logs and inspired by design choices used by
dune for user messages:

  • [Pp_tty] extends pp to build colored documents in the user's
    terminal using ansi escape codes.

  • [Err] is an abstraction to report located errors and warnings to
    the user.

  • [Log] is an interface to logs using [Pp_tty] rather than [Format].

  • [Log_cli] contains functions to work with [Err] on the side of end
    programs (such as a command line tool). It defines command line
    helpers to configure the [Err] library, while taking care of setting
    the logs and fmt style rendering.

  • [Cmdlang_cmdliner_runner] is a library for running command line
    programs specified with cmdlang with cmdliner as a backend and
    making opinionated choices, assuming your dependencies are using
    [Err].

These libraries are meant to combine nicely into a small ecosystem of
useful helpers to build CLIs in OCaml.



🐫 Pull-request generated by opam-publish v2.5.1

@Zeta611
Copy link

Zeta611 commented May 22, 2025

Thanks for the contribution @mbarbin! The CI failures seem to be for flambda variants (again), and other than that, it looks good to me. Can you confirm @shonfeder?

@mbarbin
Copy link
Contributor Author

mbarbin commented May 22, 2025

Thanks @Zeta611 - In case this helps : I have no time pressure to release, if we prefer to think a bit more about these CI failures and whether to try some amends on the package side, it is fine with me. Thank you.

@shonfeder
Copy link
Contributor

Looking the CI results more carefully, I think the reason we are seeing a CI failure in your packagss on flambda (and not in any other packages I've seen), is because of how you are configuring the warning behavior in your builds?

The error is

[ERROR] The compilation of pplumbing.0.0.13 failed at "dune build -p pplumbing -j 71 @install".

#=== ERROR while compiling pplumbing.0.0.13 ===================================#
# context              2.3.0 | linux/x86_64 | ocaml-options-only-flambda.1 | pinned(https://github.com/mbarbin/pplumbing/releases/download/0.0.13/pplumbing-0.0.13.tbz)
# path                 ~/.opam/4.14/.opam-switch/build/pplumbing.0.0.13
# command              ~/.opam/opam-init/hooks/sandbox.sh build dune build -p pplumbing -j 71 @install
# exit-code            1
# env-file             ~/.opam/log/pplumbing-7-e8d3ae.env
# output-file          ~/.opam/log/pplumbing-7-e8d3ae.out
### output ###
# (cd _build/default && /home/opam/.opam/4.14/bin/ocamlopt.opt -w -40 -w +a-4-40-41-42-44-45-48-66 -warn-error +a -g -I lib/log/src/.log.objs/byte -I lib/log/src/.log.objs/native -I /home/opam/.opam/4.14/lib/logs -I /home/opam/.opam/4.14/lib/pp -I lib/pp_tty/src/.pp_tty.objs/byte -I lib/pp_tty/src/.pp_tty.objs/native -intf-suffix .ml -no-alias-deps -o lib/log/src/.log.objs/native/log.cmx -c -impl lib/log/src/log.ml)
# File "_none_", line 1:
# Error (warning 58 [no-cmx-file]): no cmx file was found in path for module Stdune__User_message, and its interface was not compiled with -opaque

And we see that this is actually just a warning. From Drup has noted that

This is a warning, not an error, and you can safely ignore it.

It is here to tell you that the compiler might miss optimizations (since it needs .cmx to do inlining) ... but you really don't care about optimizations for the ocamlbuild plugin, so it's fine.

But you have configured -warn-error +a, which makes all warnings as fatal errors.

IIUC

  • to prevent the warnings from occurring, you can submit issues or patches to the offending libraries, as Drup recommends, to ensure they are incuding .cmx files.
  • to allow more flexible installation, I would advise not making all warnings errors in your builds. This is useful in development, but IMO probably not the behavior we'd want in releases? Given that, afaiu, dune already treats warnings as errors in dev mode do you actually need to be setting that flag?

I am OK with merging this as is, but would you like to try fixing the build rules so people could install this using flambda in ocaml 4.14 (and perhaps in other context where warnings might occur)?

@mbarbin
Copy link
Contributor Author

mbarbin commented May 22, 2025

  • to allow more flexible installation, I would advise not making all warnings errors in your builds. This is useful in development, but IMO probably not the behavior we'd want in releases? Given that, afaiu, dune already treats warnings as errors in dev mode do you actually need to be setting that flag?

Thanks a lot, this is really helpful. My attention wasn't on the warnings, and this is new information for me. I'll take some time to reflect on this. This particular setup is pretty pervasive on my side, but it doesn't mean I can't adjust. Let's hold on for, as I consider some amends on my side, experiment some more with dev mode. etc. Thanks!

@mbarbin
Copy link
Contributor Author

mbarbin commented May 26, 2025

@shonfeder I tried locally with an opam switch that has the same options as with the one failing in the CI:

ocaml-option-flambda.1,ocaml-options-only-flambda.1,ocaml-variants.4.14.2+options

The error was the same locally, up to a good start!

File "_none_", line 1:
Error (warning 58 [no-cmx-file]): no cmx file was found in path for module Stdune__User_message, and its interface was not compiled with -opaque

I tried next to disable implicit_transitive_deps:

diff --git a/dune-project b/dune-project
index 7bb7fbe..bd24c54 100644
--- a/dune-project
+++ b/dune-project
@@ -17,7 +17,8 @@
-(implicit_transitive_deps false)
+;; CR mbarbin: Trying without this to see whether this impacts the warning.
+;; (implicit_transitive_deps false)

With this change, there was no warning generated.

So, I was not completely wrong suspecting the error had something to do with this option.

Where I am now in my understanding is that compiling with this flambda switch with ocaml-4.14 and (implicit_transitive_deps false) produces a warning that does not occur when this option is not set. And in turn, because the project has this peculiar treatment of --warn-error +a, this result in overall failure.

I am torn about what to do, still thinking a bit. On one hand I can see that not treating the warning as error would work, but this doesn't feel right to me at the moment. If anything, if I decided that I was "happy" with warning 58, I would simply disable it completely in my build options for this package (rather than letting it show to all of the packages users).

But, also, this shows some issues with (implicit_transitive_deps false) in dune. I reported this too in this dune issue.

My latest thought is that I am tempted to remove (implicit_transitive_deps false) from the main branch, and only add this during the CI, so it will monitor correct use during dev and PRs but not affect publishing to opam (I am not in favor of disabling warn-errors at the moment).

@mbarbin
Copy link
Contributor Author

mbarbin commented May 26, 2025

Closing in favor of #27924

@mbarbin mbarbin closed this May 26, 2025
@shonfeder
Copy link
Contributor

I can see that not treating the warning as error would work, but this doesn't feel right to me at the moment

In my opinion, warnings should never be treated as errors when building in the release profile,as the nature of warnings is that they should not prevent correct or safe use of a project, which is what users of a release care about.

@mbarbin
Copy link
Contributor Author

mbarbin commented May 26, 2025

I hear you. To say a few words about an opposing view, I feel there is something unsettling as a library author, about letting users run a somewhat degraded version of the library when we have no idea what that alteration might be. In some way, if a new warning shows up, I would find value in getting an opportunity to look it over (and react).

A unfair example, but if you think of unused values warning for example, in my experience they're very often the symptom of an actual bug.

As a caring user, I fear being discouraged by projects whose build output contains unattended warnings.

This may be completely untenable of a stand, I grant you that! I need time to think about it. Thanks a lot for the discussion!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants